Show Server Version in Yorkie CLI #938
Conversation
|
|
WalkthroughThe recent updates enhance the Yorkie CLI by introducing a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant YorkieCLI
participant AdminService
participant Server
User->>YorkieCLI: Request server version
YorkieCLI->>AdminService: Call GetServerVersion()
AdminService->>Server: Retrieve version info
Server-->>AdminService: Return version details
AdminService-->>YorkieCLI: Send version info
YorkieCLI-->>User: Display version info
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
api/yorkie/v1/admin.pb.gois excluded by!**/*.pb.goapi/yorkie/v1/resources.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- admin/client.go (1 hunks)
- api/docs/yorkie/v1/admin.openapi.yaml (5 hunks)
- api/docs/yorkie/v1/resources.openapi.yaml (1 hunks)
- api/types/version_info.go (1 hunks)
- api/yorkie/v1/admin.proto (2 hunks)
- api/yorkie/v1/resources.proto (2 hunks)
- api/yorkie/v1/v1connect/admin.connect.go (9 hunks)
- cmd/yorkie/version.go (1 hunks)
- server/rpc/admin_server.go (2 hunks)
- server/rpc/server_test.go (1 hunks)
- server/rpc/testcases/testcases.go (1 hunks)
Additional context used
golangci-lint
cmd/yorkie/version.go
27-27: File is not
goimports-ed with -local github.com/yorkie-team/yorkie(goimports)
Additional comments not posted (23)
api/types/version_info.go (2)
3-10: LGTM!The
VersionInfostruct is well-defined and includes fields for both client and server versions.
12-22: LGTM!The
VersionDetailstruct is well-defined and includes fields for version details.api/yorkie/v1/admin.proto (2)
46-46: LGTM!The
GetServerVersionRPC method is well-defined and follows best practices for gRPC services.
171-175: LGTM!The
GetServerVersionResponsemessage type is well-defined and includes fields for version details.cmd/yorkie/version.go (4)
35-118: LGTM!The
newVersionCmdfunction is well-defined and follows best practices for Cobra commands.
124-131: LGTM!The
Validatefunction is well-defined and follows best practices for validation functions.
133-139: LGTM!The
getYorkieClientVersionfunction is well-defined and follows best practices for version functions.
140-157: LGTM!The
initfunction is well-defined and follows best practices for initialization functions.server/rpc/server_test.go (1)
214-216: LGTM! Verify the implementation ofRunAdminGetServerVersionTest.The new test case is approved.
However, ensure that the
RunAdminGetServerVersionTestfunction is correctly implemented.Verification successful
The implementation of
RunAdminGetServerVersionTestis correct.The function correctly retrieves the server version and asserts that there are no errors and the response is not nil.
server/rpc/testcases/testcases.golines 1019-1030Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `RunAdminGetServerVersionTest`. # Test: Search for the function implementation. Expect: Correct implementation of the function. ast-grep --lang go --pattern $'func RunAdminGetServerVersionTest($_, $_) { $$$ }'Length of output: 861
api/yorkie/v1/resources.proto (2)
22-22: LGTM!The new import statement for
google/protobuf/empty.protois approved.
380-380: LGTM!The new message type
Emptyis approved.admin/client.go (1)
357-370: LGTM! Verify the usage oftypes.VersionDetail.The new method
GetServerVersionis approved.However, ensure that the
types.VersionDetailstruct is correctly defined and used.Verification successful
Verify the usage of
types.VersionDetail.The
types.VersionDetailstruct is correctly defined in./api/types/version_info.go. Now, let's ensure it is used appropriately throughout the codebase.
Verified: The
types.VersionDetailstruct is correctly defined and used.
- The
types.VersionDetailstruct is properly defined with the necessary fields.- It is consistently and correctly used across the codebase, including in the
GetServerVersionmethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `types.VersionDetail`. # Test: Search for the struct definition. Expect: Correct definition and usage of the struct. ast-grep --lang go --pattern $'type VersionDetail struct { $$$ }'Length of output: 630
Script:
#!/bin/bash # Description: Verify the usage of `types.VersionDetail` throughout the codebase. # Test: Search for all instances where `types.VersionDetail` is used. rg 'VersionDetail' -A 5Length of output: 2014
server/rpc/admin_server.go (1)
440-450: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetServerVersionare correctly integrated.Verification successful
Function usage verified and correctly integrated.
The
GetServerVersionfunction is used across multiple files, including test cases, client implementations, and generated protobuf files. All instances appear to be correctly integrated.
server/rpc/server_test.goserver/rpc/testcases/testcases.gocmd/yorkie/version.goadmin/client.goapi/yorkie/v1/admin.pb.goapi/yorkie/v1/v1connect/admin.connect.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetServerVersion` are correctly integrated. # Test: Search for the function usage. Expect: Only occurances of the new function. rg --type go -A 5 $'GetServerVersion'Length of output: 12410
api/yorkie/v1/v1connect/admin.connect.go (5)
107-107: LGTM!The method signature for
GetServerVersionis correctly added to theAdminServiceClientinterface.
296-296: LGTM!The method signature for
GetServerVersionis correctly added to theAdminServiceHandlerinterface.
276-279: LGTM!The method implementation for
GetServerVersionis correctly added to theadminServiceClientstruct.
370-374: LGTM!The handler for the
GetServerVersionmethod is correctly added to theNewAdminServiceHandler.
466-468: LGTM!The method stub for
GetServerVersionis correctly added to theUnimplementedAdminServiceHandler.server/rpc/testcases/testcases.go (1)
1018-1030: LGTM! But verify the test function usage in the codebase.The test function
RunAdminGetServerVersionTestis correctly implemented.However, ensure that the test function is correctly integrated and executed in the test suite.
Verification successful
The test function
RunAdminGetServerVersionTestis correctly integrated and executed in the test suite.
server/rpc/server_test.go: The test function is invoked here, ensuring it is part of the test execution.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test function usage in the codebase. # Test: Search for the test function usage. Expect: Only occurances of the new test function. rg --type go -A 5 $'RunAdminGetServerVersionTest'Length of output: 837
api/docs/yorkie/v1/resources.openapi.yaml (1)
376-380: LGTM! Theyorkie.v1.Emptyschema is well-defined.The schema correctly represents an empty object with
additionalPropertiesset tofalse, ensuring no unexpected properties are included.api/docs/yorkie/v1/admin.openapi.yaml (3)
61-72: LGTM! The new endpoint/yorkie.v1.AdminService/GetServerVersionis well-defined.The endpoint correctly uses the POST method with an empty request body and a detailed response schema.
219-227: LGTM! The schemayorkie.v1.AdminService.GetServerVersion.yorkie.v1.Emptycorrectly referencesyorkie.v1.Empty.This ensures consistency and clarity in the API documentation.
799-819: LGTM! The schemayorkie.v1.GetServerVersionResponseis well-defined.The schema includes appropriate fields for
buildDate,goVersion, andyorkieVersion, withadditionalPropertiesset tofalse.
There was a problem hiding this comment.
The code in the PR generally looks good, but there are a few edge cases that need to be addressed. Currently, the behavior varies in different scenarios:
A. When the CLI has an authenticated context:
The user can log in, view contexts, and see both client and server version information.
$ yorkie login -u admin -p admin --insecure
$ yorkie context ls
CURRENT RPC ADDR INSECURE TOKEN
* localhost:8080 true eyJhbGciOi...hxtu4u7_jE
$ yorkie version
Yorkie Client: 0.4.27
Go: go1.22.1
Build Date: 2024-07-23
Yorkie Server: 0.4.27
Go: go1.22.1
Build Date: 2024-07-23B. When the CLI has no context:
The user can see client version information, but receives an error when trying to fetch the server version.
$ yorkie logout
$ yorkie version
Yorkie Client: 0.4.27
Go: go1.22.1
Build Date: 2024-07-23
Error fetching server version: auth for localhost:8080 does not exist
C. When the CLI requests version information from an old server:
The user sees client version information but gets an authentication error for the server version.
$ yorkie version --rpc-addr api.yorkie.dev:443
Yorkie Client: 0.4.27
Go: go1.22.1
Build Date: 2024-07-23
Error fetching server version: auth for api.yorkie.dev:443 does not exist
D. Questions and suggestions for improvement:
Q) Should we allow users to view the server version without authentication? This could be beneficial for troubleshooting and version compatibility checks.
Q) How should we handle different scenarios?
- When the server endpoint is incorrect or the server is not running: We should provide a clear error message indicating connection issues.
- When the server is running an older version without the version API: We should gracefully handle this case, perhaps by showing the client version and indicating that the server version is unavailable or outdated.
|
First of all, thank you very much for your review. @hackerwins
A) I agree that checking the server version without authentication has several advantages. So, I would like to add a flag that can check the version of a specific addr without authentication. For example, options like
A) Thank you. If the server version is not loaded properly, I need to modify it to display a more appropriate error message.
A) Likewise, we need to show an appropriate message regarding the case you mentioned. For example, Phrases like: 'server version checking is only supported if the server version is 0.4.27 or higher.' Lastly, there was an error in my code. I tried to use auth, but there was a problem with only loading auth that does not fit the current context because Thank you :) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
api/yorkie/v1/admin.pb.gois excluded by!**/*.pb.go
Files selected for processing (6)
- admin/client.go (1 hunks)
- api/docs/yorkie/v1/admin.openapi.yaml (4 hunks)
- api/yorkie/v1/admin.proto (2 hunks)
- api/yorkie/v1/v1connect/admin.connect.go (9 hunks)
- server/rpc/admin_server.go (2 hunks)
- server/rpc/testcases/testcases.go (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- admin/client.go
- api/docs/yorkie/v1/admin.openapi.yaml
- api/yorkie/v1/admin.proto
- api/yorkie/v1/v1connect/admin.connect.go
- server/rpc/admin_server.go
- server/rpc/testcases/testcases.go
It would be better to remove authentication entirely for checking the server version. Version is often considered public information and it improves the experience for developers who need to quickly check version. I am also curious about whether kubectl's version command checks authentication. 🤔 |
That's a relief, I asked just in case. Now I understood your intentions correctly. However, there is one thing that concerns me. I don't know much about security, but I wonder if there will be any security issues due to server version exposure. 🤔 Additionally, |
|
When thinking about who will be using this feature, I think the internal developers who combined Yorkie with their system will use this feature to check or troubleshoot something. So I'm not sure about the necessity of exposing this feature to public.
@hyun98 I think there can be a malicious attack by penetrating the weakness of the certain server version exposed by this feature. |
hyun98
force-pushed
the
feature/#870-show-server-version-in-yorkie-cli
branch
from
9990f06
to
742d6d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
api/yorkie/v1/admin.pb.gois excluded by!**/*.pb.go
Files selected for processing (9)
- admin/client.go (1 hunks)
- api/docs/yorkie/v1/admin.openapi.yaml (4 hunks)
- api/types/version_info.go (1 hunks)
- api/yorkie/v1/admin.proto (2 hunks)
- api/yorkie/v1/v1connect/admin.connect.go (9 hunks)
- cmd/yorkie/version.go (1 hunks)
- server/rpc/admin_server.go (2 hunks)
- server/rpc/server_test.go (1 hunks)
- server/rpc/testcases/testcases.go (1 hunks)
Files skipped from review due to trivial changes (1)
- api/types/version_info.go
Files skipped from review as they are similar to previous changes (5)
- admin/client.go
- api/yorkie/v1/admin.proto
- cmd/yorkie/version.go
- server/rpc/server_test.go
- server/rpc/testcases/testcases.go
Additional comments not posted (11)
api/yorkie/v1/v1connect/admin.connect.go (6)
115-115: LGTM!The method signature in the
AdminServiceClientinterface is correctly defined.
328-328: LGTM!The method signature in the
AdminServiceHandlerinterface is correctly defined.
306-309: LGTM!The method implementation in the
adminServiceClientstruct is correctly defined.
520-522: LGTM!The method implementation in the
UnimplementedAdminServiceHandlerstruct is correctly defined.
412-416: LGTM!The handler for the
GetServerVersionRPC is correctly defined.
449-450: LGTM!The HTTP handler for the
GetServerVersionRPC is correctly defined.api/docs/yorkie/v1/admin.openapi.yaml (5)
85-96: LGTM!The endpoint for retrieving the server version is correctly defined.
261-269: LGTM!The request body schema for retrieving the server version is correctly defined.
415-423: LGTM!The response schema for retrieving the server version is correctly defined.
901-905: LGTM!The schema for the
GetServerVersionRequestis correctly defined.
906-926: LGTM!The schema for the
GetServerVersionResponseis correctly defined.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- cmd/yorkie/version.go (1 hunks)
- test/bench/tree_editing_bench_test.go (1 hunks)
Files skipped from review due to trivial changes (1)
- test/bench/tree_editing_bench_test.go
Additional comments not posted (6)
cmd/yorkie/version.go (6)
20-33: Imports LGTM!The new imports are necessary for the added functionality.
37-40: Variable Declarations LGTM!The new variables
clientOnlyandoutputare necessary for the added functionality.
41-137: FunctionnewVersionCmdLGTM with minor suggestions.The logic for validation and concurrent processing is well-implemented. However, consider the following improvements:
- Error Handling: Enhance error handling by providing more detailed messages for different error cases.
- Code Readability: Improve code readability by breaking down the function into smaller helper functions.
Example refactor for error handling:
func handleServerError(cmd *cobra.Command, serverErr error) { cmd.Printf("Error fetching server version: ") connectErr := new(connect.Error) if errors.As(serverErr, &connectErr) { switch connectErr.Code() { case connect.CodeUnimplemented: cmd.Printf("The server does not support this operation. You might need to check your server version.\n") } } else { cmd.Print(serverErr) } }And then call
handleServerError(cmd, serverErr)in place of the current error handling logic.
144-151: FunctionValidateLGTM!The function correctly validates the output format.
153-159: FunctiongetYorkieClientVersionLGTM!The function correctly returns the client version details.
160-177: FunctioninitLGTM!The function correctly adds the new version command and sets up the flags.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #938 +/- ##
==========================================
+ Coverage 51.31% 51.34% +0.02%
==========================================
Files 71 71
Lines 10679 10685 +6
==========================================
+ Hits 5480 5486 +6
Misses 4650 4650
Partials 549 549 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for your contribution 😄
I have left some comments regarding the code style and etc.
FYI: After this PR merges, you might have to update corresponding Yorkie CLI Docs for this CLI feature.
|
Thanks for your comment. |
- remove goroutine has no advantage - method extract for readability
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
cmd/yorkie/version.go (1)
73-93: Consider enhancing error handling ingetServerVersion.While the function correctly retrieves the server version, consider adding more context to error messages for better debugging.
- return nil, err + return nil, fmt.Errorf("failed to get server version: %w", err)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- admin/client.go (1 hunks)
- cmd/yorkie/version.go (1 hunks)
- server/rpc/testcases/testcases.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- admin/client.go
- server/rpc/testcases/testcases.go
Additional comments not posted (8)
cmd/yorkie/version.go (8)
20-32: Imports look good.The imports are appropriate for the functionalities implemented in this file.
36-39: Variable declarations are appropriate.The variables
clientOnlyandoutputare well-named and serve their intended purpose.
40-67: FunctionnewVersionCmdis well-structured.The function correctly sets up the version command with validation and error handling.
95-101: FunctiongetClientVersionis correct.The function correctly returns the client version details.
103-121: FunctionprintServerErroris adequate with a TODO for improvement.The function handles server errors and includes a TODO for future improvements.
124-150: FunctionprintVersionInfois well-implemented.The function provides flexible output options for version information.
153-160: FunctionValidateOptionsis correct.The function correctly validates the output format options.
161-178:initfunction is well-structured.The function correctly initializes the version command with appropriate flags.
This commit adds the functionality to display the version of connected Yorkie server in the version command. The version information can be viewed by converting it into yaml or json format using the --output, -o flag. This flag accepts json and yaml strings and converts them to the appropriate format for display. In order to solely check the version of the client CLI without considering the server's version, the --client flag has been included. The development of this feature was inspired by examining the kubectl.
This commit adds the functionality to display the version of connected Yorkie server in the version command. The version information can be viewed by converting it into yaml or json format using the --output, -o flag. This flag accepts json and yaml strings and converts them to the appropriate format for display. In order to solely check the version of the client CLI without considering the server's version, the --client flag has been included. The development of this feature was inspired by examining the kubectl.
This commit adds the functionality to display the version of connected Yorkie server in the version command. The version information can be viewed by converting it into yaml or json format using the --output, -o flag. This flag accepts json and yaml strings and converts them to the appropriate format for display. In order to solely check the version of the client CLI without considering the server's version, the --client flag has been included. The development of this feature was inspired by examining the kubectl.
This commit adds the functionality to display the version of connected Yorkie server in the version command. The version information can be viewed by converting it into yaml or json format using the --output, -o flag. This flag accepts json and yaml strings and converts them to the appropriate format for display. In order to solely check the version of the client CLI without considering the server's version, the --client flag has been included. The development of this feature was inspired by examining the kubectl.
What this PR does / why we need it:
Add the functionality to display the version of connected Yorkie server in the
versioncommandWhich issue(s) this PR fixes:
Fixes #870
Special notes for your reviewer:
yamlorjsonformat using the--output,-oflag.This flag accepts
jsonandyamlstrings and converts them to the appropriate format for display.kubectlcode.Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests